feat: promisify all api methods that accept callbacks #381
Conversation
This is a stop-gap until the full async/await migration can be completed. It means we can refactor tests of other modules that depend on this module without having to mix async flow control strategies. N.b. some methods that were previously callable without callbacks (e.g. `node.start()`, `node.stop()`, etc) now require callbacks otherwise a promise is returned which, if rejected, can cause `unhandledPromiseRejection` events and lead to memory leaks.
|
We need to update the readme for this. Since @achingbrain is out, I'll go ahead and update that. |
|
I went ahead and added a generic note at the top of the API section about promisify support and the move to async/await. I think it makes sense to update the API code once we do the actual switch over here, in case there are any changes to the way the api behaves. |
src/pubsub.js
Outdated
| if (!node.isStarted() && !floodSub.started) { | ||
| return nextTick(callback, errCode(new Error(messages.NOT_STARTED_YET), codes.PUBSUB_NOT_STARTED)) | ||
| } | ||
| if (!handler && !callback) { | ||
| if (!callback) { |
There was a problem hiding this comment.
This is going to be a breaking change. Currently js-ipfs calls self.libp2p.pubsub.unsubscribe(topic, handler), which will end up resulting in this calling handler as a callback. We'll need to do this as a minor release and then update js-ipfs.
There was a problem hiding this comment.
Yeah, I was aiming to have this and gossipsub added for a new minor release
There was a problem hiding this comment.
Yes, that would be ideal to release them together. I think I could also do #384 (pulling in the core modules) as part of the same minor.
The unfortunate thing about this logic is that if you want to unsubscribe a single handler, you have to use callbacks, otherwise we assume the handler is a callback.
There was a problem hiding this comment.
For #384 we will also have to get the tests for those modules here, as well as probably refactor some stuff, as I believe we will end up with a lot of replicated stuff. So, unless you feel that #384 will not take a long time, I think we should get those 2, as they are kind of ready to go. And we can unblock gossipsub on js-libp2p and js-ipfs, and more async migration work.
yeah, I see it. This is a tricky case because of the 2 last arguments with a function
There was a problem hiding this comment.
I'll open a PR for 384 and if it's not ready by then, we can release without it. I don't want to hold anything up with.
A different route we could go is to return Promise.resolve() if callback is not defined. The caveat of this approach is that if you want to unsubscribe from everything, you have to provide a null or undefined handler, but you don't have to use callbacks ever.
There was a problem hiding this comment.
hmm, it is more coherent with the API to do what you suggest. But, we need to have proper docs for help on that
There was a problem hiding this comment.
I think we can note the difference in the readme, and maybe add an example of the calls to the jsdocs for the method to help. The docs currently point to the ipfs interface for the api, which describes the calls as one of:
await libp2p.unsubscribe(topic)
await libp2p.unsubscribe(topic, handler)
libp2p.unsubscribe(topic, handler, callback)as the code currently is, the first two calls are the same through promisify. With my recommended changes we'd temporarily have the following until we go full async/await:
await libp2p.unsubscribe(topic, null)
await libp2p.unsubscribe(topic, handler)
libp2p.unsubscribe(topic, handler, callback)There was a problem hiding this comment.
Yeah, I think we can go with that approach
There was a problem hiding this comment.
@vasco-santos I am thinking of opening an additional PR to fix the subscribe method since we're doing a breaking change. Right now, the order of params (topic, options, handler, callback) does not match the interface api, it would be nice to correct that with this release. I can add jsdocs for it and the rest of the pubsub methods in that PR. What do you think?
|
@vasco-santos this is ready for one last review pass and should be good to merge. I had to fix some tests due to the pubsub change and a recent release of the delegate content router. |
This fixes a bug where `ADD_PROVIDER` message keys were CID bytes instead of multihash bytes, and changes the CIDs generated in tests to be V1s with `raw` codec instead of V0s so that the difference is actually tested. Fixes libp2p#381
## [6.0.4](libp2p/js-libp2p-kad-dht@v6.0.3...v6.0.4) (2022-12-07) ### Bug Fixes * use multihash bytes for provide message keys ([libp2p#405](libp2p/js-libp2p-kad-dht#405)) ([d7e7b5d](libp2p/js-libp2p-kad-dht@d7e7b5d)), closes [libp2p#381](libp2p/js-libp2p-kad-dht#381)
This is a stop-gap until the full async/await migration can be completed. It means we can refactor tests of other modules that depend on this module without having to mix async flow control strategies.
N.b. some methods that were previously callable without callbacks (e.g.
node.start(),node.stop(), etc) now require callbacks otherwise a promise is returned which, if rejected, can causeunhandledPromiseRejectionevents and lead to memory leaks where exceptions are thrown and resources are not cleaned up.